-
Notifications
You must be signed in to change notification settings - Fork 95
Remote server support #1423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Remote server support #1423
Conversation
pkg/registry/data/registry.json
Outdated
@@ -4166,5 +4166,93 @@ | |||
"transport": "stdio" | |||
} | |||
}, | |||
"remote_servers": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These remote servers are currently added only for testing purposes. I’m not suggesting adding them permanently to our registry.
If we decide to include them, I can ensure that all tools and associated metadata are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no biggie.
@@ -59,13 +60,25 @@ ToolHive supports four ways to run an MCP server: | |||
Runs an MCP server using a previously exported configuration file. | |||
5. Remote MCP server: | |||
$ thv run --remote <URL> [--name <name>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the --remote
flag really needed? Can't we just determine that it should be a remote server by the fact that the entry is a URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can avoid --remote
flag, it was added as the initial design for remote servers
cmd.Flags().StringVar(&config.RemoteAuthAuthorizeURL, "remote-auth-authorize-url", "", | ||
"OAuth authorize URL for remote server authentication") | ||
cmd.Flags().StringVar(&config.RemoteAuthTokenURL, "remote-auth-token-url", "", | ||
"OAuth token URL for remote server authentication") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are shared with the proxy
subcommand, right? Would it make sense to have a common function to set these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we can make them common.
// Handle image retrieval | ||
imageURL, imageMetadata, err := handleImageRetrieval(ctx, serverOrImage, runFlags) | ||
imageURL, imageMetadata, remoteServerMetadata, err := handleImageRetrieval(ctx, serverOrImage, runFlags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would make more sense to make this a two step process:
- Determine whether a registry entry is remote or not
- use the appropriate entry for the server... this will be a different function call depending on it being remote or not.
@@ -324,17 +397,29 @@ func buildRunnerConfig( | |||
rt runtime.Deployer, | |||
imageURL string, | |||
imageMetadata *registry.ImageMetadata, | |||
remoteServerMetadata *registry.RemoteServerMetadata, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about using the ServerMetadata
interface instead?
https://github.com/stacklok/toolhive/blob/main/pkg/registry/types.go#L185
This would allow you to only pass one value here and from there you can determine whether it's a remote entry or not.
@@ -723,7 +723,7 @@ func (s *WorkloadRoutes) getWorkloadNamesFromRequest(ctx context.Context, req bu | |||
// createWorkloadFromRequest creates a workload from a request | |||
func (s *WorkloadRoutes) createWorkloadFromRequest(ctx context.Context, req *createRequest) (*runner.RunConfig, error) { | |||
// Fetch or build the requested image | |||
imageURL, imageMetadata, err := retriever.GetMCPServer( | |||
imageURL, imageMetadata, _, err := retriever.GetMCPServer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about returning ServerMetadata
here instead?
@@ -0,0 +1,345 @@ | |||
// Package discovery provides authentication discovery utilities for detecting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could have been in a separate PR. It's fine now, but if you do want to expedite this I'd suggest that still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically the existing functionality, just refactored it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Dynamic client registration, I would create a separate PR
Timeout: 30 * time.Second, | ||
} | ||
|
||
return client.Do(newReq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this HTTP client doesn't use authentication at all... how is this used? I'm a little confused by this tbh.
func (t *tracingTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
if t.p.isRemote { | ||
// Use manual HTTP client instead of reverse proxy for remote URLs | ||
resp, err := t.manualForward(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't using manual forwarding break dual communication channels like SSE?
// For remote servers, strip the /mcp path since they expect requests at the root | ||
if p.isRemote && strings.HasPrefix(r.URL.Path, "/mcp") { | ||
// Strip /mcp from the path for remote servers | ||
r.URL.Path = strings.TrimPrefix(r.URL.Path, "/mcp") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't that be a matter of not using /mcp when doing the request?
Support Remote MCP Servers with OAuth/OIDC Authentication
Overview
This PR adds support for running remote MCP servers using the
thv run --remote
command, similar to how local MCP servers are run. Remote MCP servers are treated as workloads managed via runconfigs, enabling full functionality such as client configuration, tool filtering, and import/export.Key Features
1. Remote Server Support
thv run --remote <URL of MCP server>
thv list
command2. OAuth/OIDC Authentication
--remote-auth-client-id
and--remote-auth-client-secret
--remote-auth-authorize-url
and--remote-auth-token-url
3. Registry Support
Usage Examples
Basic Remote Server
OAuth/OIDC Authentication
Registry-Based Remote Servers
Server Management
Technical Implementation
Authentication Flow
.well-known/openid_configuration
OIDC Discovery
.well-known/openid_configuration
and.well-known/oauth-authorization-server
authorization_endpoint
andtoken_endpoint
Registry Integration
RemoteServerMetadata
struct with OAuth configurationauthorize_url
,token_url
,scopes
,oauth_params
Client Configuration
localhost:port
)Configuration
Registry Schema
Remote servers are defined in
pkg/registry/data/registry.json
:RunConfig Structure
Limitations
Next Steps